Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UFO Rountrip #244

Merged
merged 44 commits into from
Mar 20, 2018
Merged

UFO Rountrip #244

merged 44 commits into from
Mar 20, 2018

Conversation

belluzj
Copy link
Collaborator

@belluzj belluzj commented Oct 19, 2017

This will have the code the for the UFO roundtrip as per #235

@belluzj belluzj force-pushed the real-ufo-roundtrip branch 3 times, most recently from aae464a to 1e09c3f Compare October 30, 2017 17:39
@belluzj
Copy link
Collaborator Author

belluzj commented Oct 31, 2017

Here is an update on this work: the simple parts do roundtrip on GlyphsUnitTestSans, i.e. the outlines, features, kerning, classes, glyphs, layers, font attributes, custom parameters, alignment zones, guidelines, anchors, hints, annotations. There are still some small problems here and there, that you can see in this diff between the original GlyphsUnitTestSans () and the same after conversion to UFO and back to Glyphs ()

The tricky parts that are not done yet are:

  • Smart components: the piece data and the PartsSelection data are not copied, and more importantly the smart components are not interpolated in the UFO output
  • instance data and bracket layers: the instance data and interpolated instances are not persisted in my tests, although the code exists, and there is no handling of bracket layers

As for the testing, I rely on the builder being thoroughly tested to be sure that the UFO output is usable by UFO application and on the writer being thoroughly tested to ensure that the textual diff between .glyphs files is an accurate representation of what is in memory. If those two components are indeed well tested, then the roundtrip tests can just be textual diffs on files that cover all the features.

I have a few questions:

  1. There is a GSBackgroundLayer class in the code that is not mentioned in the documentation. Why?
  2. The glyph order is always written to UFO, but when it is read back, I would like to persist it in the glyphs file only if it is different from the default Glyphs.app ordering. Does that make sense? How do I know whether the current order is the default one?
  3. Is it "OK" to have a layer with no name in Glyphs? It is possible to create one in the python console. What would it mean and how should it be handled? In the code, there currently is a condition that says "if it the layer has no name, don't store it to UFO".
  4. Is there a preferred way of ordering entries in the kerning dictionary in .glyphs files? For the glyphs->GSFont->.glyphs roundtrip, there was an OrderedDict to preserve the order from the file. For the UFO roundtrip I used a simple dict to enforce alphabetical ordering of dict keys and get a nicer textual diff. Could it be possible for Glyphs.app to also output kerning rules in alphabetical order of keys? Is there another "computable" order?
  5. In order to roundtrip customParameters and don't mix them with other user data, and know whether they came from the GSFont or the GSFontMaster, I have used longer keys like "com.schriftgestaltung.customParameter.GSFontMaster.My Custom Parameter". I'm not sure that it's a good idea. Does that look ok? Do you have suggestions?
  6. To go from UFO .fea to GSFont::features, I used feaLib to parse the fea and snip the relevant code fragments. I have written some code to add information to the feaLib AST nodes (i.e. both ends of the span of text that made an AST node instead of just the beginning). Should I upstream that to feaLib?

@anthrotype
Copy link
Member

Thanks for the update, Jany!

I wouldn't worry too much about the smart components or bracket layers at this stage. Let's get the basic stuff working first.

If those two components are indeed well tested, then the roundtrip tests can just be textual diffs

sgtm

GSBackgroundLayer... why?

No idea. @schriftgestalt ?

The glyph order is always written to UFO

that's because the way Glyphs.app orders things without an explicit glyphOrder custom parameter is different from the way ufo2ft would order if glyphOrder is not present. The latter simply places .notdef at the beginning and sorts the rest alphabetically. I believe Glyphs.app uses the order in which the glyphs are written out in the *.glyphs file itself, which in turn seems to follow the order of the "Categories" lists on the left panel. Should we ask Georg to share that list? I don't know. Maybe it's ok if after roundtripping, one always gets an extra glyphOrder custom parameter (provided that the UFO libs has glyphOrder value).

a preferred way of ordering entries in the kerning dictionary in .glyphs files?

actually I don't know. Is it not alphabetic?

In order to roundtrip customParameters... I have used longer keys

that's ok. explicit is better than implicit

I have written some code to add information to the feaLib AST nodes... Should I upstream that to feaLib?

Sure, thanks!

@schriftgestalt
Copy link
Collaborator

schriftgestalt commented Oct 31, 2017

  1. There is a GSBackgroundLayer class in the code that is not mentioned in the documentation. Why?

it is a subclass of GSLayer. The most important difference is the handling of .background and .foreground.

  1. The glyph order is always written to UFO, but when it is read back, I would like to persist it in the glyphs file only if it is different from the default Glyphs.app ordering. Does that make sense? How do I know whether the current order is the default one?

Glyphs always writes the public.glyphOrder and, if present a com.schriftgestaltung.glyphOrder. Only the later is read back in.

  1. Is it "OK" to have a layer with no name in Glyphs? It is possible to create one in the python console. What would it mean and how should it be handled? In the code, there currently is a condition that says "if it the layer has no name, don't store it to UFO".

Master layers don't have a name on there own. They get the name from the master. For other layers, it is possible but not advised.

  1. Is there a preferred way of ordering entries in the kerning dictionary in .glyphs files? For the glyphs->GSFont->.glyphs roundtrip, there was an OrderedDict to preserve the order from the file. For the UFO roundtrip I used a simple dict to enforce alphabetical ordering of dict keys and get a nicer textual diff. Could it be possible for Glyphs.app to also output kerning rules in alphabetical order of keys? Is there another "computable" order?

I would not like to change that just like that. But I see your point.

  1. In order to roundtrip customParameters and don't mix them with other user data, and know whether they came from the GSFont or the GSFontMaster, I have used longer keys like "com.schriftgestaltung.customParameter.GSFontMaster.My Custom Parameter". I'm not sure that it's a good idea. Does that look ok? Do you have suggestions?

Glyphs stores font.customParameters in com.schriftgestaltung.font.+parameterName = value that is not the smartest idea, as there are parameters that can occur more than once with the same name. I should change it to com.schriftgestaltung.font.customParameters = {"name": parameterName, "value": parameterValue}, (or something like this). And do the same for the master.

@anthrotype
Copy link
Member

Glyphs always writes the public.glyphOrder and, if present a com.schriftgestaltung.glyphOrder. Only the later is read back in.

what's the difference between the two?

Could it be possible for Glyphs.app to also output kerning rules in alphabetical order of keys?

I would not like to change that just like that. But I see your point.

if not alphabetical, what's the order then? The glyphOrder's order?

there are parameters that can occur more than once

yes, that's why for filters, for example, we write it as a list of dicts.

Copy link
Collaborator

@moyogo moyogo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s what I noted GlyphsApp does:
.glyphs -> .ufo:

  • the glyph order in the font is written to public.glyphOrder.
  • if present, the customParameter glyphOrder is written to com.schriftgestaltung.glyphOrder

.ufo -> .glyphs:

  • public.glyphOrder defines the order of the glyphs in the font.
  • com.schriftgestaltung.glyphOrder is written to the customParameter glyphOrder if it is not false and if there was none public.glyphOrder is written to the customParameter glyphOrder instead.

It’s seems hard to roundtrip with the same logic since .ufo -> .glyphs -> .ufo will change public.glyphOrder and add com.schriftgestaltung.glyphOrder.

We probably want to rething the logic so we can roundtrip but preserve the extra information.

register(ParamHandler(
glyphs_name, ufo_name, glyphs_long_name=ufo_name,
value_to_ufo=lambda value: -abs(value),
value_to_glyphs=abs,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In UFO openTypeOS2WinDescent must be non-negative, this should probably be

        value_to_ufo=abs,
        value_to_glyphs=abs,

@@ -16,151 +16,497 @@
unicode_literals)

import re
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re is not used here anymore.

for c in layer.components:
value = getattr(c, key)
# FIXME: (jany) bad idea to skip none because it shifts indices
# TODO: (jany) switch to a dict structure: {componentId: {data}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing the locked state of components as a dict {componentId: {data}} would be a lot better than a list of booleans indeed.

@schriftgestalt Would it make sense for GlyphsApp to use that for UFO3 since components can have identifiers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to distinguish between the public.glyphOrder and the con.schriftgestaltung.glyphOrder? Isn’t the former enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicating the glyphOrder is not only confusing but may lead to the two values getting out of sync with each other

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense for GlyphsApp to use that for UFO3 since components can have identifiers.

I’ll follow any working solution.

Why is it necessary to distinguish between the public.glyphOrder and the con.schriftgestaltung.glyphOrder? Isn’t the former enough?

They mean different things. The public.glyphOrder stores the order of the glyphs as the happened to be at that moment. con.schriftgestaltung.glyphOrder is just the custom parameter, It doesn't need to be in sync with the actual order and can contain names that are not in the font and doesn’t need to have all glyphs. So you should mostly be using public.glyphOrder and just keep con.schriftgestaltung.glyphOrder around for round tripping.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way you described the com.schriftgestaltung.glyphOrder sounds to me very much like the way the UFO spec defines the public.glyphOrder. The fact is the UFO glyph sets don’t have any “actual order” as contents.plist is just a mapping without any internal order (it’s alphabetically sorted in most implementations but doesn’t imply anything).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are differences.
Normally, Glyphs will sort the glyphs automatically. That can be deactivated and manually controlled by the parameters. As I said, the list can be much shorter that the number of glyphs in the font. The remaining glyphs will be sorted by the internal rules.

If Glyphs opens a .ufo it looks for the ‘com.schriftgestaltung.glyphOrder’ entry. If it finds one, it set the glyphOrder parameter (if it finds a list) or does nothing if it is set to None. If the entry isn’t found, it looks for the ‘keep glyph names from imported fonts’ setting from the user defaults and if it is set, it puts the ‘public.glyphOrder’ into the custom parameter. Otherwise does nothing again. In those later cases, the glyphs will be sorted automatically.

So I need both parameters to be able to round trip properly. But as I said, glyphsLib doesn’t need to look at the prefix list.

@@ -15,6 +15,10 @@
from __future__ import (print_function, division, absolute_import,
unicode_literals)

from glyphsLib.types import point

LOCKED_NAME_SUFFIX = ' [locked]'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to store locked guidelines in the glyph.lib or for global guidelines in the font.lib like it is done for locked components, or better, as a dictionary as you propose?

/cc @schriftgestalt

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t have an opinion about this.

@belluzj
Copy link
Collaborator Author

belluzj commented Nov 16, 2017

Quick status update

At that point, the only remaining diff on GlyphsUnitTestSans is the instance data, as shown in the following screenshot.

screenshot_1

My plan is now to:

  1. resolve that remaining diff properly (see the questions in the next section)
  2. diff the UFOs produced by this branch, the UFOs from master and those from Glyphs, and for each discrepancy, find out which way is better and add builder tests to enforce that
  3. run the Glyphs->UFO->Glyphs roundtrip on Noto, add tests
  4. Also add a few UFO->Glyphs->UFO tests with custom data and lib in the UFO

Questions: how to store instance data in the designspace document

I'm currently trying to store this data into the designspace document and I feel that I'm not doing it right, because the current code I have for Glyphs -> UFO (that is based on the existing interpolation.py) is actually writing down to the disk all the master UFOS, plus the designspace document. The code uses mutatorMath DesignSpaceDocumentWriter.

When I want to read back the instance data for UFO -> Glyphs, I'm using mutatorMath DesignSpaceDocumentReader and its readInstances method, which writes to the disk all the UFO instances, so that finally I can read back instance data from the UFO's lib

All those disk operations for instance data are to be compared to the fully in-memory process that I had for everything else while doing Glyphs->UFO->Glyphs. In a way it's good because writing the intermediary UFOs to disk highlighted some problems, so for testing it's nice to write to the disk, but I think that in general the software should be able to operate in memory.

Is that indeed considered a problem or is it just me? Would that other library for DesignSpaceDocument improve the situation? https://github.com/LettError/designSpaceDocument
I saw that there is a plan to integrate it into fontTools: fonttools/fonttools#911
Should that happen before I start using it in glyphsLib, so that I don't have to add another dependency here?

@anthrotype
Copy link
Member

thanks Jany for the update, and for the great work you're doing!
The plan sounds good to me

how to store instance data in the designspace document

Have you seen this?
LettError/designSpaceDocument#19

you shouldn't need to build instance UFOs and save them to disk just to read back from them the instance data. The instance data (including the custom parameters) should be stored inside the designspace document, and anything that doesn't already fit should be placed in a residual new lib plist element, similar to UFO's and GLIF's lib. So you'll only need the set of master UFOs plus their designspace file to completely define a corresponding *.glyphs file.

I think that in general the software should be able to operate in memory

yes, I agree.

Would that other library for DesignSpaceDocument improve the situation?

probably. I ping'ed Erik about it

LettError/designSpaceDocument#28

Should that happen before I start using it in glyphsLib, so that I don't have to add another dependency here?

would you like to do that? I mean, work on copying (along with its git history) the designSpaceDocument/__init__.py to a new module inside fontTools package, also called designSpaceDocument.
That would be great :)

@belluzj
Copy link
Collaborator Author

belluzj commented Nov 16, 2017

The instance data (including the custom parameters) should be stored inside the designspace document

I agree and that's what I wanted to do, but the mutatorMath APIs don't seem to allow it.

OK then, first, I will use designSpaceDocument as a dependency here to keep moving, and I will open a PR in fontTools to move the file and its git history.

@belluzj belluzj force-pushed the real-ufo-roundtrip branch 2 times, most recently from a43f1d8 to 5defdea Compare November 24, 2017 16:07
@belluzj belluzj force-pushed the real-ufo-roundtrip branch 2 times, most recently from c7823a2 to ca35f8b Compare December 13, 2017 15:27
@belluzj belluzj force-pushed the real-ufo-roundtrip branch 4 times, most recently from 200d54e to bc5f5b9 Compare February 1, 2018 16:41
@belluzj belluzj force-pushed the real-ufo-roundtrip branch 3 times, most recently from e0c8f78 to db88a07 Compare February 16, 2018 15:50
WIP UFO round-trip

WIP Glyphs and layers

WIP kerning, hints, annotations

WIP kerning, features

WIP path, components

WIP paths, features, kerning, glyph order

WIP features, alignment zones, guidelines

WIP Fix layer ordering

WIP restore backround layers, anchor positions, path node order

WIP custom parameters

WIP Glyph, Layer, Node user data

WIP layers

WIP background image

WIP fix some tests

Fixup custom_params

Fixup builder.glyph

Fixup builder.guidelines

WIP GSBackgroundLayer, glyphOrder

WIP small fixes, refactor types.py

WIP small fixes

WIP smart component data

WIP instances

WIP roundtrip instances using designspace HELP IT'S AWFUL

WIP use designSpaceDocument for round-trip

WIP implement InMemorySourceDescriptor and test for writing on disk

WIP instances
@belluzj
Copy link
Collaborator Author

belluzj commented Feb 19, 2018

This branch is getting ready for merge: I won't add other features, but I will cleanup some TODOs/FIXMEs that are not relevant anymore, update some docstrings and try to spot regressions. Also I'm waiting on fonttools/fonttools#1175 to drop my copy-pasted version of DesignSpaceDocument :)

About regressions, I have a question: I have tested manually the generation of instance TTFs using fontmake on one of the Noto fonts, to check that the TTFs come out the same as with the current master. To be more confident, I would like to build all of Noto with master, get all the files as TTXs, do the same with this branch, and compare the TTXs (they should be the same except for the timestamp). So, question: is there already some automated regression testing for the Noto fonts?


Apart from that, here are a few examples of how to use the new UFO->Glyphs feature. I have added those to the readme:

  1. Without a designspace file, using for example the Inria fonts by Black[Foundry]:
import glob
from defcon import Font
from glyphsLib import to_glyphs
ufos = [Font(path) for path in glob.glob('*Italic.ufo')]
# Sort the UFOs because glyphsLib will create masters in the same order
ufos = sorted(ufos, key=lambda ufo: ufo.info.openTypeOS2WeightClass)
font = to_glyphs(ufos)
font.save('InriaSansItalic.glyphs')

Here is the resulting glyphs file

  1. With a designspace, using Spectral from Production Type:
import glob
from fontTools.designspaceLib import DesignSpaceDocument
from glyphsLib import to_glyphs
doc = DesignSpaceDocument()
doc.read('spectral-build-roman.designspace')
font = to_glyphs(doc)
font.save('SpectralRoman.glyphs')

Here is the resulting glyphs file

  1. In both cases, if you intend to go back to UFOs after modifying the file with Glyphs, you should use the minimize_ufo_diffs parameter to minimize the amount of diffs that will show up in git after the back and forth. To do so, the glyphsLib will add some bookkeeping values in various userData fields. For example, it will try to remember which GSClass came from groups.plist or from the feature file.

The same option exists for people who want to do Glyphs->UFOs->Glyphs: minimize_glyphs_diffs, which will add some bookkeeping data in UFO lib. For example, it will keep the same UUIDs for Glyphs layers, and so will need to store those layer UUIDs in the UFOs.

import glob
import os
from fontTools.designspaceLib import DesignSpaceDocument
from glyphsLib import to_glyphs, to_designspace, GSFont
doc = DesignSpaceDocument()
doc.read('spectral-build-roman.designspace')
font = to_glyphs(doc, minimize_ufo_diffs=True)
doc2 = to_designspace(font, propagate_anchors=False)
# UFOs are in memory only, attached to the doc via `sources`
# Writing doc2 over the original doc should generate very few git diffs (ideally none)
doc2.write(doc.path)
for source in doc2.sources:
    path = os.path.join(os.path.dirname(doc.path), source.filename)
    # You will want to use ufoNormalizer after
    source.font.save(path)

font = GSFont('SpectralRoman.glyphs')
doc = to_designspace(font, minimize_glyphs_diffs=True, propagate_anchors=False)
font2 = to_glyphs(doc)
# Writing font2 over font should generate very few git diffs (ideally none):
font2.save(font.filepath)

In practice there are always a few diffs on things that don't really make a difference, like optional things being added/removed or whitespace changes or things getting reordered...

@belluzj belluzj changed the title WIP UFO Rountrip UFO Rountrip Mar 19, 2018
@belluzj
Copy link
Collaborator Author

belluzj commented Mar 20, 2018

Hello! This PR should be merged soon because it's become very big and I have started incorporating fixes that are out of the scope of "going from UFOs to glyphs".

We have started using this branch internally at Dalton Maag to work on UFOs with Glyphs and it works good enough.

I'm pretty sure that I have introduced some regressions in the Glyphs -> UFOs -> TTFs pipeline, but I would be happy to fix them once this is merged and people start using this code.

@moyogo @anthrotype Are you OK with this?

@anthrotype
Copy link
Member

Hey Jany, thanks. Yeah I was discussing this with Denis at Robothon, and I also think we should merge before it diverges too much.

I'm pretty sure that I have introduced some regressions in the Glyphs -> UFOs -> TTFs

Why? what makes you think so?

@belluzj
Copy link
Collaborator Author

belluzj commented Mar 20, 2018

Just "statistically": there are so many changes that I have probably broken something. For example, I suspect that there might be problems with how custom parameters are stored in UFO lib and used again later in the toolchain by ufo2ft or fontmake. That's why I was asking about Noto non-regression testing a few days ago, to hunt for such regressions.

@anthrotype
Copy link
Member

Noto non-regression testing

not yet, but we'll be working on that.

OK, let's merge to master and hold the release a few more days, while I try to rebuild all the Noto fonts compare them with notodiff/fontdiff.

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @belluzj for working on this! 👍

I think I'm gonna need some help to draft the changelog for this ;)

@belluzj belluzj merged commit cc41f34 into googlefonts:master Mar 20, 2018
@anthrotype
Copy link
Member

I just bumped the minor version (2.3.0.dev0), as I presume there aren't no backward compatibility changes, only new features.

To all glyphsLib users, please try this out and let us know if you spot any regressions! Thanks 🙌

I could tag a pre-release (beta, or release candidate) and publish wheels to make it easier for other users to test this out with pip install --upgrade --pre glyphsLib.

Or maybe it's fine to just install from git

pip install -e git+https://github.com/googlei18n/glyphsLib.git#egg=glyphsLib

@@ -326,15 +327,17 @@ def set_width_class(ufo, designspace, instance):
_set_class_from_instance(ufo, designspace, instance, WIDTH_AXIS_DEF)


def apply_instance_data(designspace):
# DEPRECATED: needs better API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@belluzj what do you propose to use as an alternative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a working replacement now so maybe I should not mark this as deprecated but only as "todo"? The idea behind the deprecated is:

I think the apply_instance_data should work as independently from glyphsLib as possible. I marked the current function as "deprecated" because it takes as a parameter something that is produced by glyphsLib only, whereas I think it should take only a designspace and find everything it needs in it.

Actually, in the code here, the parameter "instance_data" is already only stuff from the designspace, but weirdly wrapped in something that looks like a dictionary to conform to expectations of existing users of the API. I think there should be a new API that more clearly exposes the intent, which is:

  • inputs:
    • a designspace file that specifies instances
    • interpolated UFOs for those instances, at the locations pointed by each instance.path of the designspace
  • outputs:
    • each existing UFO is updated according to special Glyphs.app customParameters/filters/whatever, whose values/configuration were found in the instance.lib of the designspace (and only there, no need to read the glyphs file again).

Also from a broader perspective I guess this feature of applying post-processing to interpolated instance UFOs will be needed as well in workflows that are not based on Glyphs, so I guess this function could be part of something else than glyphsLib, and implement postprocessing in a similar way as ufo2ft filters (the code of Glyphs.app-specific filters could be provided by glyphsLib though). It could also be done by MutatorMath or the "ufoProcessor" from the designspaceDocument repository, I'm not sure which is suitable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Yeah, I don't like the interface of that function for the same reasons, but any changes will need to be coordinated with fontmake.
It'd be nice to make this feature generic and non-glyphsLib specific. Probably mutatorMath (or ufoProcessor if/when it's fully feature-compatible with the latter) is a better place to define this API than is ufo2ft. The latter doesn't know about designspace yet, it's only concerned with converting UFOs to OTFs.

@belluzj belluzj mentioned this pull request Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants